GH-51029: [C++] Fix MapArray validation with unknown null count - #51093
Conversation
|
|
pitrou
left a comment
There was a problem hiding this comment.
Thanks for doing this @AnuragRaut08 ! See comments below.
Also, there are other uses of MayHaveNulls in array_nested.cc, shouldn't we fix them as well?
Reranko05
left a comment
There was a problem hiding this comment.
Also fix the lint by using:-
python -m pre_commit run clang-format --files <file_path>
0e4c2a2 to
4574e70
Compare
|
Done, ran clang-format on the affected C++ files with pre-commit. |
|
@pitrou I checked the other MayHaveNulls() uses in array_nested.cc. They are used for validating nullable list/map offsets when constructing arrays, rather than validating MapArray keys, so I left them unchanged. |
But the same issue applies: |
4574e70 to
aee42e9
Compare
|
Thanks, addressed. I have updated the remaining |
| MANIFEST | ||
| compile_commands.json | ||
| build.ninja | ||
| build/ |
|
@AlenkaF FYI |
Rationale for this change
MapArrayvalidation usesMayHaveNulls()to check that the map child and keys contain no nulls. When an all-valid validity bitmap is present with an unknown null count,MayHaveNulls()can report that the array may contain nulls even though all values are valid.During a cast of a
MapArray, this can cause validation to return an invalid status which is passed toARROW_CHECK_OK, resulting in a process abort instead of a recoverable error.What changes are included in this PR?
GetNullCount()instead ofMayHaveNulls()when validating theMapArraychild and keys.kUnknownNullCount.Are these changes tested?
Yes, with existing and additional unit tests.
Are there any user-facing changes?
Only a bugfix.
This PR contains a "Critical Fix".
This fixes a bug that can cause a process crash when casting a valid
MapArraywhose keys carry an all-valid validity bitmap with an unknown null count. This is easily reproduced in Python: